Skip to content

CLN use pd.Timestamp for single element in converter #49148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 18, 2022

Conversation

MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Oct 17, 2022

Only a single element is being parsed, and there's no way for a user to pass any options (like dayfirst) through ax.set_xlim, so might as well use pd.Timestamp.

Otherwise with #49024 users might get a warning they wouldn't be able to do anything about

Example of test that hits this: pytest pandas/tests/plotting/test_datetimelike.py -k test_time_change_xlim

@@ -152,7 +152,7 @@ def _to_ordinalf(tm: pydt.time) -> float:

def time2num(d):
if isinstance(d, str):
parsed = tools.to_datetime(d)
parsed = Timestamp(d)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably changes error messages? If yes, should add a whatsnew

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this also makes the check/raise on the next two lines unnecesary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good call, looks like they're uncovered as it is anyway https://app.codecov.io/gh/pandas-dev/pandas/blob/main/pandas/plotting/_matplotlib/converter.py

@phofl phofl added this to the 2.0 milestone Oct 18, 2022
@phofl phofl merged commit 2e7f5a3 into pandas-dev:main Oct 18, 2022
@phofl
Copy link
Member

phofl commented Oct 18, 2022

thx @MarcoGorelli

noatamir pushed a commit to noatamir/pandas that referenced this pull request Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants